Skip to content

[firebase_dynamic_links] make sure the initial link resolves#2580

Closed
hffmnn wants to merge 4 commits into
firebase:masterfrom
howfm:feature/initial-link-resolving-ios
Closed

[firebase_dynamic_links] make sure the initial link resolves#2580
hffmnn wants to merge 4 commits into
firebase:masterfrom
howfm:feature/initial-link-resolving-ios

Conversation

@hffmnn

@hffmnn hffmnn commented May 18, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

We found that sometimes the initial link might not resolve correctly. This is because of a possible race condition:

  1. iOS calls
- (BOOL)application:(UIApplication *)application
    continueUserActivity:(NSUserActivity *)userActivity
      restorationHandler:(void (^)(NSArray *))restorationHandler

and link resolving starts by calling onInitialLink.

  1. handleMethodCall is called with @"FirebaseDynamicLinks#getInitialLink" while the initial link still gets resolved
  2. Because _initialLink is still nil that gets send via the channel.

To resolve this issue, this PR

  1. sets the new BOOL isResolvingInitialLink to YES while resolving.
  2. checks if isResolvingInitialLink is YES when @"FirebaseDynamicLinks#getInitialLink" gets called and waits until it is NO.

Not sure if this solution is too simplistic, but at least it worked for our case.

Related Issues

#1624
#100

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@hffmnn hffmnn requested a review from bparrishMines as a code owner May 18, 2020 16:55
@stephenhuey stephenhuey mentioned this pull request May 25, 2020
Closed
@degloff degloff mentioned this pull request Oct 9, 2020
Closed
@degloff

degloff commented Oct 9, 2020

Copy link
Copy Markdown

@hffmnn thanks for that fix. I tested and seems to work. I tested with a custom deep link and also
with Google Auth email link signin. Both works now when the app is closed. Initial link is resolved properly.

As this is a recurring issue open now for a year can someone with the authority press the merge button? Thanks.

@hffmnn

hffmnn commented Oct 9, 2020

Copy link
Copy Markdown
Contributor Author

@degloff Good to hear that it works for you.

can someone with the authority press the merge button

Would need quite some conflict resolution and some retesting because master changed quite a bit.

@degloff

degloff commented Oct 9, 2020

Copy link
Copy Markdown

@hffmnn actually I don't think it is too much of an issue, your changes are just in FLTFirebaseDynamicLinksPlugin.m and that did not change too much. But better if someone looks at it again with good ObjectiveC knowledge.

@koskimas

koskimas commented Oct 9, 2020

Copy link
Copy Markdown

I think there's a misunderstanding of what getInitialLink is supposed to do. It's not supposed to wait for the initial link as this PR assumes and does. It's supposed to return the link that may or may not have been delivered before the onLink listener was installed.

The actual app start link (initial link) can be delivered to either onLink or get returned by getInitialLink as stated in the documentation. Maybe this fact could be made more clear in the docs though.

@degloff

degloff commented Oct 9, 2020

Copy link
Copy Markdown

@koskimas Thanks for the clarification. However I exactly tested the ^0.6.0+2 version of the plugin by first registering onLink handlers and then call getInitialLink. However, this does not solve the problem either and the link that opened the closed app, neither was received by onLink nor by getInitialLink. Actually to be precise, in about 1 out of 10 cases it was received by getInitialLink. As we use email link signup and other deep link quite intensively, this is a very bad end user experience.

The downside of @hffmnn implementation is of course that the startup phase of the app shows the white screen for a little longer before the first screen can be rendered.

@hffmnn hffmnn force-pushed the feature/initial-link-resolving-ios branch from 384e058 to 5b6382c Compare March 19, 2021 17:09
@Salakar

Salakar commented Apr 21, 2021

Copy link
Copy Markdown
Contributor

Closing in favour of #4354

@Salakar Salakar closed this Apr 21, 2021
@firebase firebase locked and limited conversation to collaborators May 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@bparrishMines bparrishMines Awaiting requested review from bparrishMines
@Ehesp Ehesp Awaiting requested review from Ehesp
@kroikie kroikie Awaiting requested review from kroikie
@Salakar Salakar Awaiting requested review from Salakar

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hffmnn @degloff @koskimas @Salakar @googlebot